GH-47447: [C++] Fix replace_with_mask for null type arrays by AnkitAhlawat7742 · Pull Request #49950 · apache/arrow

AnkitAhlawat7742 · 2026-05-08T16:24:32Z

Rationale for this change

replace_with_mask crashes when called with null type arrays because a code path incorrectly returns Status::OK() from a function with return type Result<int64_t>.
This PR fixes the issue by ensuring the function returns a valid int64_t value instead of Status::OK() for successful execution.

What changes are included in this PR?

Fixed the replace_with_mask kernel implementation for null type arrays.
Replaced the invalid Status::OK() return path with a valid int64_t result.
Prevented construction of Result<int64_t> with an OK Status.

Are these changes tested?

Yes, manually run the test case

Are there any user-facing changes?

No

This PR contains a "Critical Fix".

This change fixes a crash in replace_with_mask when operating on null type arrays. The crash occurs due to an invalid successful return path internally constructing Result with Status::OK().

GitHub Issue: [C++][Python] replace_with_mask crashes when null type inputs are used #47447

github-actions · 2026-05-08T16:24:56Z

⚠️ GitHub issue #47447 has been automatically assigned in GitHub to PR creator.

…enario to test

AnkitAhlawat7742 · 2026-05-12T11:53:40Z

Hi @raulcd ,
Could you please re-run the failed job? i see download build timed out while downloading the ORC dependency (orc-2.2.1.tar.gz)

 --- LOG END ---
          error: downloading 'https://dlcdn.apache.org/orc/orc-2.2.1/orc-2.2.1.tar.gz' failed
          status_code: 22
          status_string: "HTTP response code said error"
          log:
          --- LOG BEGIN ---
            Trying 151.101.2.132:443...

AnkitAhlawat7742 · 2026-05-13T04:06:21Z

Hi @raulcd ,
Please review the changes ,let me know if any modification needed

pitrou

Thanks @AnkitAhlawat7742 for this PR.

Can you add some C++ tests in the existing file vector_replace_test.cc?
You can get some inspiration from the existing TestReplaceBoolean.

…p and updated the python test with null type

AnkitAhlawat7742 · 2026-05-14T04:13:09Z

Thanks @AnkitAhlawat7742 for this PR.

Can you add some C++ tests in the existing file vector_replace_test.cc? You can get some inspiration from the existing TestReplaceBoolean.

HI @pitrou ,
Thanks for the review ..!!
I have updated the python Test case and added the new test case in vector_replace_test file .and thanks for the reference of existing class

AnkitAhlawat7742 · 2026-05-15T04:57:55Z

Hi @pitrou and @raulcd ,
Please review the changes, when you get a chance. I noticed a CI failure in the Windows build but confirmed it is unrelated to my changes.

AnkitAhlawat7742 · 2026-05-19T04:18:40Z

Hi @pitrou and @raulcd,
just a gentle ping on this PR when you get a chance. please review it once again ...happy to make any changes if needed!

cc @AlenkaF @rok @jorisvandenbossche

AnkitAhlawat7742 · 2026-05-26T04:20:44Z

Hi @kou ,
Please review the PR as well

Copilot

Pull request overview

This PR fixes a crash in the C++ replace_with_mask kernel when operating on NullType arrays (GH-47447), where a Result<int64_t> path incorrectly attempted to return Status::OK(). The fix ensures the kernel returns a valid int64_t and produces a valid output for null arrays, and adds regression tests in both C++ and Python.

Changes:

Fix NullType replace_with_mask implementation to return a valid int64_t (and set output as ArrayData) instead of returning Status::OK() from a Result<int64_t> function.
Add C++ regression tests covering replace_with_mask on NullType.
Add Python regression test reproducing the original crash scenario.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
`cpp/src/arrow/compute/kernels/vector_replace.cc`	Fixes `NullType` `ReplaceMaskImpl` to return a valid `int64_t` and sets output as `ArrayData`, preventing the invalid `Result<T>(Status::OK())` construction.
`cpp/src/arrow/compute/kernels/vector_replace_test.cc`	Adds a dedicated `NullType` test fixture and `ReplaceWithMask` test cases to prevent regressions in the C++ kernel.
`python/pyarrow/tests/test_compute.py`	Adds a Python regression test for `pc.replace_with_mask` with `pa.null()` arrays to ensure the crash is fixed end-to-end.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kou

+1

…failure

AnkitAhlawat7742 · 2026-05-27T03:32:49Z

Hi @AlenkaF ,
Please review and approve the PR

kou · 2026-05-27T05:09:00Z

I'll merge this in a few days if nobody objects it.
If we find any problem in this PR after we merge this, we can fix it as a follow-up task.

zanmato1984

+1, thanks for the fix.

AlenkaF

The one ask I have, before this gets merged, is to move the tests somewhere else in the same file. Currently the PyArrow test is located between two connected tests (test_fill_null_array and test_fill_null_chunked_array). I suggest moving it after test_extract_regex_span which is connected to test_replace_regex.

Also another ask, maybe as a follow-up, would be to add tests for regular use of replace_with_mask kernel in PyArrow. AFAIU there is none currently!

AnkitAhlawat7742 · 2026-05-27T10:36:12Z

test_fill_null_array

Hi @AlenkaF ,

Thank you for the review comments!
I have moved test_replace_with_mask_null_type after test_extract_regex_span test case.
Regarding adding test cases for regular replace_with_mask usage ....I will raise a separate PR for that as a follow-up.

AlenkaF

Thanks @AnkitAhlawat7742

kou · 2026-05-28T02:40:33Z

We have multiple approvals now. I'll merge this.

AnkitAhlawat7742 · 2026-05-28T07:52:20Z

We have multiple approvals now. I'll merge this.

Thank you @kou, @AlenkaF, @zanmato1984, and @pitrou for the reviews and merging this PR! I will raise a follow-up PR to add tests for regular replace_with_mask usage in PyArrow.

conbench-apache-arrow · 2026-05-28T08:48:24Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2c1293f.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

apacheGH-47447: [C++] Fix replace_with_mask for null type arrays

498166c

github-actions Bot added Component: C++ awaiting review Awaiting review labels May 8, 2026

apacheGH-47447: [Python] Add pandas replace mask and combine_first sc…

9e8a563

…enario to test

AnkitAhlawat7742 requested review from AlenkaF, raulcd and rok as code owners May 12, 2026 04:36

github-actions Bot added the Component: Python label May 12, 2026

apacheGH-47447: [Python] Remove pandas dependency from test

2f48adb

pitrou requested changes May 13, 2026

View reviewed changes

Comment thread python/pyarrow/tests/test_compute.py Outdated

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 13, 2026

apacheGH-47447: [C++] [Python]Add newtest for replace_with_mask in cp…

46803f4

…p and updated the python test with null type

apacheGH-47447: [C++] Fix clang-format in vector_replace_test.cc

970909a

AnkitAhlawat7742 requested a review from pitrou May 26, 2026 04:21

kou requested a review from Copilot May 26, 2026 05:03

Copilot started reviewing on behalf of kou May 26, 2026 05:03 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

kou approved these changes May 26, 2026

View reviewed changes

Comment thread python/pyarrow/tests/test_compute.py Outdated

github-actions Bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels May 26, 2026

AnkitAhlawat7742 added 2 commits May 26, 2026 11:32

apacheGH-47447: [Python] update test case variable names

7fd5659

apacheGH-47447:[Python] update test case variable names and fix lint …

4403723

…failure

zanmato1984 approved these changes May 27, 2026

View reviewed changes

AlenkaF reviewed May 27, 2026

View reviewed changes

apacheGH-47447:[Python] Move test_replace_with_mask_null_type test

2828bcb

AlenkaF approved these changes May 27, 2026

View reviewed changes

kou merged commit 2c1293f into apache:main May 28, 2026
56 checks passed

kou removed the awaiting merge Awaiting merge label May 28, 2026

kou mentioned this pull request May 28, 2026

[C++][Python] replace_with_mask crashes when null type inputs are used #47447

Closed

Conversation

AnkitAhlawat7742 commented May 8, 2026 • edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented May 8, 2026

Uh oh!

AnkitAhlawat7742 commented May 12, 2026

Uh oh!

AnkitAhlawat7742 commented May 13, 2026

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AnkitAhlawat7742 commented May 14, 2026

Uh oh!

AnkitAhlawat7742 commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnkitAhlawat7742 commented May 19, 2026

Uh oh!

AnkitAhlawat7742 commented May 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AnkitAhlawat7742 commented May 27, 2026

Uh oh!

kou commented May 27, 2026

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

AlenkaF left a comment

Choose a reason for hiding this comment

Uh oh!

AnkitAhlawat7742 commented May 27, 2026

Uh oh!

AlenkaF left a comment

Choose a reason for hiding this comment

Uh oh!

kou commented May 28, 2026

Uh oh!

Uh oh!

AnkitAhlawat7742 commented May 28, 2026

Uh oh!

conbench-apache-arrow Bot commented May 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AnkitAhlawat7742 commented May 8, 2026 •

edited by kou

Loading

AnkitAhlawat7742 commented May 15, 2026 •

edited

Loading